-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[11.x] Add option to disable merging of base configuration #51579
[11.x] Add option to disable merging of base configuration #51579
Conversation
I very much support this PR or any solution which allows me to disable this merging completely. I strongly believe this "feature" should never have made it into the code base, it's diametral to "least surprise" for devs. Thank you! |
Do you mind sharing an explicit example how to do that? |
It would probably look something like below. <?php
// bootstrap/app.php
use Illuminate\Foundation\Application;
use Illuminate\Foundation\Configuration\Exceptions;
use Illuminate\Foundation\Configuration\Middleware;
$app = Application::configure(basePath: dirname(__DIR__))
// ...
->withExceptions(function (Exceptions $exceptions) {
//
})->create();
$app->dontMergeBaseConfiguration()
return $app; I guess I can clean this up a bit by adding the same method to |
Thanks for fixing the comments / formatting! |
I try not to leave comments which may bring down the vibe of a PR. Generally speaking, I believe the framework should be flexible. Especially in its runtime configuration. With that said, I would advise against using this method... The merging of base configuration is what allows you to slim your Laravel application, as well as get new options for free. As someone who has facilitated over 100,000 Laravel application upgrades, I can say confidently this is a good thing. I have helped countless developers at their wits end wasting days on a random bug that turned out to be a single missed configuration option. Using this method means you are fully responsible for maintaining all core configuration files in their entirety now and forever. |
Do you have an alternate solution? |
I think there's some missing points here for why some prefer to not merge it. As it stands now, it introduced a bunch of new environment variables, and as someone who upgraded apps back from 4.2, there's been a bunch of times that those variables were renamed. Having a static config and not mergeable removes the necessity to care about those changes on upgrade, which if you don't have that config "cloned" because you use that variable, you might not even know it changed if you don't read or missed it in the upgrade docs, and those included queue, mail, database variables. From me, that's a big no thank you, i prefer to have my variables static. If i need something new, i will put it in my config. They even split mariadb and mysql configs, do you really thing that's such a great idea to have auto merged? |
Here's my perspective: my number of installations is… But it's big, over half a decade old and daily actively changing. We need to have total full control of everything and "be in the know" of every change. That includes, on Laravel updates, we also need to check & diff and the configs (re-trigger writing default configs, review, pick the pieces which can stay, etc.). It's absolutely critical every piece of information is explicitly known. Auto-merging some base values not even going to be used but actually introducing problems just doesn't work for us. |
@mfn, can you share more detail on this? |
See the original discussion I created, where details, also from others, where shared -> #50876 Only a few selected config parts are affected by this (there's an explicit approved list of configs), but one of those is related to From that discussion, this is a good summary and also affects me:
|
@mfn, thanks. I still don't see the problem - such as an error or incorrect overwrite. What I see is more a desire for full control over the configuration. As I noted in my original comment, that's fine when you assume full responsibility. You and other commenters here may truly understand what that means. I still don't think this method is needed and will likely be a footgun for most. |
The whole footgun is fake no matter if it's merged or not. if you don't override the config, any changes to config can be breaking, if you override the config with mergeable option, it might not fully merge anyway so you lose those new options. In any case you cannot depend on the new option being present, because even without the merge disable you are not guaranteed to have those new options. |
For example, take a clean laravel installation, it already comes with configs, now the framework decides to add, say |
As someone who helped write this part, I have to say not true to your not true. 😉 I do not wish to start a debate. Again, if this is something you all want to do, great. I left my disclaimer for everyone else. |
I tested it just to be doubly sure, so I'm 100% positive on what i said unless it's misunderstood, but yeah, no point in ranting on. |
|
Addresses: #50876
Context
#47309 added defaults to various configurations which caused our tests to fail.
For example, we have an
inspect
command which scans through and connects to external APIs such asdatabase.connections
andqueue.connections
to make sure that all connections are healthy.This change broke that and other similar commands that scan the configuration and connect to them.
I've added
Illuminate\Foundation\Application::dontMergeBaseConfiguration()
which can be called during the bootstrap phase that will allow me to disable the merging of base configuration.